Skip to content

Conversation

@CChuYong
Copy link
Contributor

@CChuYong CChuYong commented Mar 10, 2025

This pull request includes fixing minor issues on PgVectorStore.doDelete().

  • handling different types of id
  • used JdbcTemplate.batchUpdate() to delete bulk of ids.

@sobychacko sobychacko self-assigned this Mar 10, 2025
@sobychacko sobychacko added this to the 1.0.0-M7 milestone Mar 10, 2025
@ilayaperumalg
Copy link
Member

@CChuYong Thanks for the PR fixing the issue. Could you add a test to validate the fix?

@CChuYong CChuYong force-pushed the fix-id-type-in-pgvector-store branch from 4715998 to ea4632b Compare March 11, 2025 13:27
@CChuYong
Copy link
Contributor Author

CChuYong commented Mar 11, 2025

@ilayaperumalg Sure! i added a test for testing bulk operations of PgVectorStore with different types of id.
711ac1d

@CChuYong CChuYong force-pushed the fix-id-type-in-pgvector-store branch from ea4632b to 711ac1d Compare March 11, 2025 13:32
@ilayaperumalg ilayaperumalg self-assigned this Mar 11, 2025
}

@Test
public void testBulkOperationWithUuidIdType() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the test. This test runs fine when running as a standalone but fails when all the tests within this class run. Looks like some race condition there. Could you check?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your review!
i figured out that getNativeClientTest also initializes vector table, so i also added code for dropping table in getNativeClientTest. fb829af

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the fix!

@CChuYong CChuYong force-pushed the fix-id-type-in-pgvector-store branch from 711ac1d to fb829af Compare March 12, 2025 07:44
@ilayaperumalg
Copy link
Member

@CChuYong Thanks for your contribution! Rebased and merged as 268248b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants